test-float-parse: Use an overrideable random seed#152598
test-float-parse: Use an overrideable random seed#152598tgross35 wants to merge 3 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
tgross35
force-pushed
the
test-float-parse-updates
branch
from
e82bcb2 to
5939d5d
Compare
This comment has been minimized.
This comment has been minimized.
tgross35
force-pushed
the
test-float-parse-updates
branch
from
5939d5d to
b99607f
Compare
|
The new version of indicatif pulls in a new wasm dep tree, which we don't really need. Looks like they made these optional in 0.18.4 but it hasn't been released yet console-rs/indicatif#766, so can probably just wait for that. |
This comment has been minimized.
This comment has been minimized.
| fn main() { | ||
| // Forward the opt level so we can warn if the tests are going to be slow. | ||
| let opt = env::var("OPT_LEVEL").expect("OPT_LEVEL unset"); | ||
| let profile = env::var("PROFILE").expect("PROFILE unset"); |
There was a problem hiding this comment.
Do we need to tell Cargo that we read these environment variables, or is that assumed?
There was a problem hiding this comment.
I've always assumed that these trigger a rerun, but I actually have no idea. Asked about this at rust-lang/cargo#16800.
| const SEED: [u8; 32] = *b"3.141592653589793238462643383279"; | ||
| const SEED_ENV: &str = "TEST_FLOAT_PARSE_SEED"; | ||
|
|
||
| /// Seed for tests that use a deterministic RNG, and its b64 representation for printing. Taken |
There was a problem hiding this comment.
Maybe save the dependency and just hex-dump? Should be fairly easy with something like {:#032x}{:032x} with u128...
There was a problem hiding this comment.
Great idea, I did this.
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
added
S-waiting-on-author
tgross35
force-pushed
the
test-float-parse-updates
branch
from
b99607f to
e7183bf
Compare
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Currently we warn if `debug_assertions` is set, but this isn't always accurate because debug assertions can be on at higher optimization levels. Forward some information from the build script and print it to tell a better story, and update the warning to act on opt-level and profile rather than `debug_assertions`.
Currently `test-float-parse` is mostly random tests which take a fixed seed, meaning the same set of values get tested for each invocation. This isn't ideal because we don't get the true benefit of randomness, which is to have better coverage over time. Improve this by using a randomly generated seed, which can also be set via env. The seed is printed at the beginning of each run so it is easy to reproduce failures using the same test set, if needed. Typically it isn't great to have fuzzing randomness in tests that get run in CI because it can lead to spurious failures. However, this is a test for which failure should never happen becasue the algorithms are reasonably well proven, so if one does occur it represents a very unexpected bug that needs to be addressed. The error message is updated to strongly recommend reporting before retrying and to include details on how to reproduce.
tgross35
force-pushed
the
test-float-parse-updates
branch
from
e7183bf to
32a2893
Compare
4 participants
Currently
test-float-parseis mostly random tests which take a fixedseed, meaning the same set of values get tested for each invocation.
This isn't ideal because we don't get the true benefit of randomness,
which is to have better coverage over time.
Improve this by using a randomly generated seed, which can also be set
via env. The seed is printed at the beginning of each run so it is easy
to reproduce failures using the same test set, if needed.
Typically it isn't great to have fuzzing randomness in tests that get
run in CI because it can lead to spurious failures. However, this is a
test for which failure should never happen becasue the algorithms are
reasonably well proven, so if one does occur it represents a very
unexpected bug that needs to be addressed. The error message is updated
to strongly recommend reporting before retrying and to include details
on how to reproduce.
Additionally there are two other commits with less notable changes,
details are in the messages there.